Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Implement Sandbox notifications processor and publisher #595

Merged
merged 21 commits into from
Aug 9, 2023
Merged

Implement Sandbox notifications processor and publisher #595

merged 21 commits into from
Aug 9, 2023

Conversation

Future-Outlier
Copy link
Member

@Future-Outlier Future-Outlier commented Jul 27, 2023 •
edited
Loading

TL;DR

Implement notifications processor and publisher for Sandbox.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Add new SandboxProcessor and SandboxPublisher that implement the Processor interface.

I add config in yaml file like this

notifications:
  type: sandbox
  emailer:
    emailServerConfig:
      apiKeyEnvVar: "SENDGRID_API_KEY"
    subject: "Notice: Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\"."
    sender: "[email protected]"  # Consider using a generic email instead of personal
    body: >
      Execution \"{{ name }}\" has {{ phase }} in \"{{ domain }}\". View details at
      <a href=\http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}>
      http://example.com/projects/{{ project }}/domains/{{ domain }}/executions/{{ name }}</a>. {{ error }}

screenshot
image

Tracking Issue

Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue

fixes https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Future Outlier and others added 7 commits July 27, 2023 22:42
1st version of email publisher
8061ef2 
Signed-off-by: Future Outlier <[email protected]>
fix input
af73e9a 
Signed-off-by: Future Outlier <[email protected]>
email feature
870f382 
Signed-off-by: Future Outlier <[email protected]>
@flyte-bot @eapolinario
Update boilerplate version (flyteorg#589)
89ccfd2 
Signed-off-by: Flyte-Bot <[email protected]>
Co-authored-by: eapolinario <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
@flyte-bot
Update boilerplate version (flyteorg#594)
66fb45d 
Signed-off-by: Flyte-Bot <[email protected]>
Co-authored-by: flyte-bot <[email protected]>
Signed-off-by: Future Outlier <[email protected]>
sandbox email publisher and processor with test
b8706bb 
Signed-off-by: Future Outlier <[email protected]>
delete unnecessary comments
c4a0fcf 
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier Future-Outlier mentioned this pull request Jul 27, 2023
Closed
2 tasks
Future Outlier added 3 commits July 28, 2023 21:43
@codecov
Copy link

codecov bot commented Jul 29, 2023 •
edited
Loading

Codecov Report

Merging #595 (3721b9c) into master (04bcb15) will increase coverage by 1.84%.
The diff coverage is 100.00%.

❗ Current head 3721b9c differs from pull request most recent head 19aef7f. Consider uploading reports for the commit 19aef7f to get more accurate results

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   58.42%   60.26%   +1.84%     
==========================================
  Files         168      170       +2     
  Lines       16393    13433    -2960     
==========================================
- Hits         9578     8096    -1482     
+ Misses       5964     4486    -1478     
  Partials      851      851
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
pkg/async/notifications/factory.go 19.07% <100.00%> (+14.62%) ⬆️
...notifications/implementations/sandbox_processor.go 100.00% <100.00%> (ø)
...notifications/implementations/sandbox_publisher.go 100.00% <100.00%> (ø)

... and 154 files with indirect coverage changes

remove emailer setting in factory.go and add mock emailer test in san…
e5c12bf 
…dbox_processor_test.go

Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

@pingsutw I've updated my test, please check it.
Also to use the mock emailer int test function, I've added a "return nil" in my sandbox_processor.go.
Thanks a lot.

@Future-Outlier
Copy link
Member Author

Future-Outlier commented Jul 30, 2023 •
edited
Loading

I am not pretty sure whether use a noop emailer or a mock emailer, please get in touch with me if it is necessary to change the code.
Thanks a lot.

update file go.mod and go.sum
be74e6e 
Signed-off-by: Future Outlier <[email protected]>
@Future-Outlier
Copy link
Member Author

BTW, I just copy the master branch's go.mod and go.sum to my branch currently and run "go mod tidy"
I found that it is not needed to update the go.mod and go.sum.
Thanks a lot

pingsutw
pingsutw previously approved these changes Aug 6, 2023
View reviewed changes
katrogan
katrogan previously approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@katrogan katrogan left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. This is failing the code coverage check, but I think the code cov annotations have suggestions for some additional test cases we could use to bump the code coverage on this PR and so that we can test more scenarios

@Future-Outlier
Copy link
Member Author

Looks great. This is failing the code coverage check, but I think the code cov annotations have suggestions for some additional test cases we could use to bump the code coverage on this PR and so that we can test more scenarios

Thanks for the suggestions, I've learned a lot!

@Future-Outlier Future-Outlier dismissed stale reviews from katrogan and pingsutw via c2a9301 August 9, 2023 03:01
pingsutw
pingsutw previously approved these changes Aug 9, 2023
View reviewed changes
katrogan
katrogan approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding all the unit tests!

@katrogan katrogan merged commit 478b64c into flyteorg:master Aug 9, 2023
14 checks passed
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
@Future-Outlier
Implement Sandbox notifications processor and publisher (#595)
029c295
@dyu-bot dyu-bot mentioned this pull request Sep 18, 2023
Closed
2 tasks
This was referenced Oct 30, 2023
Open
Closed
@Future-Outlier Future-Outlier mentioned this pull request May 19, 2024
Open
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@katrogan katrogan katrogan approved these changes

@EngHabu EngHabu Awaiting requested review from EngHabu EngHabu is a code owner

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@pmahindrakar-oss pmahindrakar-oss Awaiting requested review from pmahindrakar-oss pmahindrakar-oss is a code owner

@hamersaw hamersaw Awaiting requested review from hamersaw hamersaw is a code owner

@eapolinario eapolinario Awaiting requested review from eapolinario eapolinario is a code owner

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@pingsutw pingsutw Awaiting requested review from pingsutw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Future-Outlier @katrogan @wild-endeavor @pingsutw @flyte-bot